Skip to content

connect: don't answer our own cluster update with another state update - #1741

Open
ralph wants to merge 2 commits into
librespot-org:devfrom
ralph:break-connect-state-echo-loop
Open

connect: don't answer our own cluster update with another state update#1741
ralph wants to merge 2 commits into
librespot-org:devfrom
ralph:break-connect-state-echo-loop

Conversation

@ralph

@ralph ralph commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

While an active Connect device, librespot answers its own cluster update with another state
update, and the reply comes back as another cluster update. The result is a state-update loop
that runs at the round-trip period and only stops when Spotify answers 429 Too Many Requests.

The loop

Every notify() PUTs the device state. Spotify pushes the resulting cluster update back down
the dealer socket — including the one our own PUT just caused — and handle_cluster_update
sets update_state = true for any update that arrives while we are active, without checking
where it came from:

} else if self.connect_state.is_active() {
    // fixme: workaround fix, because of missing information why it behaves like it does
    //  background: when another device sends a connect-state update, some player's position de-syncs
    //  tried: providing session_id, playback_id, track-metadata "track_player"
    self.update_state = true;
}

That schedules another notify() after UPDATE_STATE_DELAY, which PUTs, which echoes. Traced
against a real session it runs at a median of 418 ms:

10:20:33.829  cluster update: Ok(DEVICE_STATE_CHANGED) from spotifly_53879, active device: spotifly_53879
10:20:34.110  Requesting …/connect-state/v1/devices/spotifly_53879
10:20:34.249  cluster update: Ok(DEVICE_STATE_CHANGED) from spotifly_53879, active device: spotifly_53879
10:20:34.528  Requesting …/connect-state/v1/devices/spotifly_53879
10:20:34.673  cluster update: Ok(DEVICE_STATE_CHANGED) from spotifly_53879, active device: spotifly_53879
10:20:34.945  Requesting …/connect-state/v1/devices/spotifly_53879

The loop has no brake of its own. Across two captured sessions, every burst ended in a
429 — five out of five. The only gaps not preceded by one are the idle stretch before playback
starts. The requests it burns are the device-state PUTs, so the ones dropped are the ones that
tell Spotify the device exists and what it is playing.

The change

ClusterUpdate.devices_that_changed already carries what is needed to tell the cases apart,
and handle_cluster_update already logs it a few lines above. Skip the update when it names
only us:

let is_own_echo = matches!(reason, Ok(ClusterUpdateReason::DEVICE_STATE_CHANGED))
    && matches!(
        cluster_update.devices_that_changed.as_slice(),
        [changed_device_id] if changed_device_id == self.session.device_id()
    );

The fixme workaround stays. The comment records that session id, playback id and track
metadata were all tried against the underlying de-sync and none helped, which is evidence
against removing it. Anything that is not our own echo still reaches it.

Kept deliberately narrow, in two ways that matter:

  • Reason-gated. "Names only us" implies "caused by us" for a state change, not for a
    device appearing, disappearing, or changing volume. Without this gate the change swallows a
    DEVICE_VOLUME_CHANGED echo that should reach the workaround — which happened in testing,
    so this is not hypothetical.
  • Exact single-element match. An empty list, or one naming this device alongside another,
    falls through to the workaround unchanged.

Commands from other devices were never affected either way: those arrive on the connect-state
request stream and set update_state from handle_connect_state_request.

Verification

Measured on macOS against a real Spotify account, comparing device-state PUTs in the 60
seconds after PlayerEvent::Playing:

PUTs in first 60 s 429s in session
before 75 2
after 1 0

Across a whole 2m38s session after the change: 17 device-state PUTs, 11 echoes suppressed, no
429 in a scenario that produced them on every previous run.

Legitimate state updates still go out — the count is small, not zero.

The workaround's case still works. With a second device sending pause, resume and volume
while librespot was the active device, the position did not de-sync:

05:18:35.799  handling: 'endpoint: pause' from c077d34a96…
05:18:35.848  PlayerEvent::Paused  at 112197ms
05:18:41.212  handling: 'endpoint: resume' from c077d34a96…
05:18:41.213  PlayerEvent::Playing at 112197ms

Paused and resumed on the same millisecond, held flat across the state updates in between, and
tracked real time correctly afterwards. Volume propagated in both directions.

On the premise

The filter assumes an echo names only the device that caused it. Checked against 449 cluster
updates captured from real sessions before writing anything: not one named more than a single
device.
Updates from other devices do appear while active, which is what the workaround
branch still handles.

Checks

cargo fmt --all -- --check, cargo build, cargo clippy --all-targets (zero warnings) and
cargo test --workspace (23 passed) all clean. Note that connect/ has no tests near
spirc.rs, so those are a don't-regress gate rather than coverage of this change; the
measurements above are the verification.

🤖 Generated with Claude Code

ralph and others added 2 commits August 16, 2026 07:13
Every state update we PUT comes back down the dealer socket as a cluster
update naming our own device, and `handle_cluster_update` sets
`update_state = true` for any update that arrives while we are active. So
the echo schedules another PUT, which echoes again. The period is the
round trip; measured against a Spotify Connect session it runs at roughly
420 ms and keeps going until the server answers 429 Too Many Requests. In
two captured sessions every burst ended that way — five out of five — so
the loop has no brake of its own. The requests it wastes are the device
state PUTs, the ones that tell Spotify this device exists and what it is
playing, so the ones dropped are the ones worth keeping.

`ClusterUpdate.devices_that_changed` already distinguishes the cases and
is already logged just above. Across 449 cluster updates captured from
real sessions, not one named more than a single device, and the ones
caused by our own PUT named us.

Kept as narrow as the evidence. A cluster update still reaches the
workaround unless it is a DEVICE_STATE_CHANGED naming this device and
nothing else: "named only us" implies "caused by us" for a state change,
not for a device appearing, disappearing or changing volume. Updates
naming us alongside another device were never observed, and they fall
through to the workaround too.

The workaround stays because the comment says it earns its place —
session id, playback id and track metadata were all tried against the
underlying de-sync. This does not touch the case it was written for.
Another device's *commands* never came through here in the first place;
they arrive on the connect-state request stream and set `update_state`
from `handle_connect_state_request`, and updates genuinely originating
from another device still set it here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 16, 2026 05:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents a state-update feedback loop in the Connect/Spirc cluster-update handling by ignoring cluster updates that are clearly echoes of this device’s own DEVICE_STATE_CHANGED PUT while the device is active.

Changes:

  • Add ClusterUpdateReason usage and detect “own echo” cluster updates via update_reason + devices_that_changed == [self.device_id].
  • Skip the existing update_state = true workaround when the cluster update is identified as an own-echo, avoiding repeated state PUTs and eventual 429 Too Many Requests.
  • Document the fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
connect/src/spirc.rs Filters out self-echo DEVICE_STATE_CHANGED cluster updates to prevent the state-update loop while preserving the existing workaround for other updates.
CHANGELOG.md Adds an unreleased changelog entry describing the loop prevention and its impact (avoids 429s).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants